src/main/actions/startBackgroundProcess.js (3)

72-120: Consider extracting lifecycle protection into a helper function.

The close handler, destroy override, and removeAllListeners override collectively form a single "protect window from destruction" concern spread across ~50 lines in the middle of startBackgroundProcess. Extracting this into a named helper (e.g., protectWindowFromDestruction(window)) would improve readability and make it testable independently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 72 - 120, Extract
the window-lifecycle protection logic into a helper named
protectWindowFromDestruction(window): move the closeHandler, its attachment, the
_preventCloseHandler assignment, the overridden destroy (using originalDestroy
and _originalDestroy), and the overridden removeAllListeners (using
originalRemoveAllListeners) into that function; have startBackgroundProcess call
protectWindowFromDestruction(backgroundWindow) before
setupIPCForwardingToBackground and before setting the global flags so behavior
is unchanged but the lifecycle-protection concern is isolated and testable.

---

72-80: event.returnValue = false is unnecessary in a close event handler.

In Electron, event.preventDefault() on the close event is sufficient to prevent the window from closing. event.returnValue is only meaningful in synchronous IPC or beforeunload handlers. Line 75 can be removed for clarity.

Proposed fix

     const closeHandler = (event) => {
       event.preventDefault();
-      event.returnValue = false;
       if (!backgroundWindow.isDestroyed()) {
         backgroundWindow.hide();
       }
-      return false;
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 72 - 80, The close
handler sets event.returnValue which is unnecessary for the Electron 'close'
event; update the closeHandler (the function assigned where backgroundWindow is
handled) to remove the line setting event.returnValue and rely solely on
event.preventDefault() and backgroundWindow.hide(); keep the checks for
backgroundWindow.isDestroyed() and the final return false if desired, but delete
the redundant event.returnValue assignment to simplify the handler.

---

103-113: Consider a less invasive approach to protecting the close handler.

Monkey-patching removeAllListeners is fragile and brittle across Electron upgrades. While removeListener is not currently patched or called in the codebase, this approach relies on incomplete protections that could be bypassed.

Instead of patching core EventEmitter methods, document that the close handler must not be removed, or refactor to guard the removal at the point where it happens (e.g., skip removing 'close' listeners in any code that calls removeAllListeners).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 103 - 113, You
currently monkey-patch backgroundWindow.removeAllListeners to protect the
closeHandler, which is fragile; instead revert that override and either (A) add
a small safe utility or guard at call sites that remove listeners so they skip
the 'close' event (e.g., replace calls to
backgroundWindow.removeAllListeners(...) with a safeRemoveAllListeners(target,
eventName) helper that delegates except when eventName is 'close' or undefined),
or (B) centralize listener cleanup in a single function that knows not to remove
the 'close' listener; update/remove the override of removeAllListeners and
ensure places that previously cleared listeners call the new safe helper or the
centralized cleanup, referencing backgroundWindow, closeHandler,
removeAllListeners, and removeListener as needed.

src/main/actions/startBackgroundProcess.js (3)

73-80: event.returnValue = false and return false are ineffective on BrowserWindow's close event.

event.preventDefault() on line 74 is the correct and sufficient way to cancel a BrowserWindow close event. event.returnValue is meaningful for webContents' beforeunload events, not BrowserWindow lifecycle events, and return false has no effect in Node EventEmitter handlers. Both can be removed to avoid misleading future readers.

Suggested cleanup

     const closeHandler = (event) => {
       event.preventDefault();
-      event.returnValue = false;
       if (!backgroundWindow.isDestroyed()) {
         backgroundWindow.hide();
       }
-      return false;
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 73 - 80, The
closeHandler currently sets event.returnValue = false and returns false which
are ineffective for a BrowserWindow 'close' event; update closeHandler to rely
solely on event.preventDefault() to cancel the close and then hide the
backgroundWindow if it's not destroyed (keep the existing
backgroundWindow.hide() call guarded by backgroundWindow.isDestroyed()); remove
the lines setting event.returnValue and the final return false so the handler
only calls event.preventDefault() and performs the hide action.

---

103-113: removeListener/off are not overridden, leaving a gap in close-handler protection.

If the intent is to prevent external code from stripping the close handler, note that removeListener('close', handler) and off('close', handler) can still remove it. Whether this matters depends on your threat model — if only removeAllListeners is called by the code paths you're guarding against, this is fine. Otherwise, consider wrapping off/removeListener similarly for the 'close' event.

Also, originalRemoveAllListeners is already .bind(backgroundWindow) on line 104, so the .call(backgroundWindow, ...) on lines 108 and 112 is redundant — you can call originalRemoveAllListeners(eventName) directly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 103 - 113, The
override only protects removeAllListeners; to fully guard the close handler on
backgroundWindow, also wrap/remove override removeListener and off so attempts
to remove the 'close' handler are ignored or re-attached: intercept
backgroundWindow.removeListener and backgroundWindow.off, check if eventName ===
'close' (and optionally match handler === closeHandler) then no-op or re-attach
closeHandler, otherwise delegate to the original bound functions; also simplify
calls to originalRemoveAllListeners by invoking
originalRemoveAllListeners(eventName) directly (remove the redundant
.call(backgroundWindow, ...)) and keep references to the originals
(originalRemoveAllListeners, originalRemoveListener, originalOff) when adding
these overrides.

---

119-120: Consider documenting the distinction between the two global flags.

global.isBackgroundProcessActive (line 119) and global.backgroundProcessStarted (line 120) convey subtly different semantics (current state vs. ever-started). A brief inline comment clarifying each flag's purpose would help future maintainers avoid misuse.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/main/actions/startBackgroundProcess.js` around lines 119 - 120, Add
concise inline comments where global.isBackgroundProcessActive and
global.backgroundProcessStarted are set (in the startBackgroundProcess flow)
explaining that isBackgroundProcessActive denotes the current run-time state
(true while the background loop/process is running) and backgroundProcessStarted
denotes an "ever-started" flag (true once the process has been initiated at
least once), so maintainers know one is transient/current-state and the other is
a one-time marker used for startup logic and metrics. Ensure comments are placed
immediately above the two assignments and mention intended usage (e.g., toggled
on stop vs. never reset) to prevent misuse.